London 11 | Pezhman-Azizi | Data-Flow | Book-Library #129
Conversation
There was a problem hiding this comment.
In terms of input validation,
- Are all input properly checked?
- Can
.valuebenull? - What if a user enters only space characters in the "title" input field?
- What if a users enters
-1or3.1416in the "pages" input field?
There are also some errors in index.html.
debugging/book-library/script.js
Outdated
| let cell1 = row.insertCell(0); | ||
| let cell2 = row.insertCell(1); | ||
| let cell3 = row.insertCell(2); | ||
| let cell4 = row.insertCell(3); | ||
| let cell5 = row.insertCell(4); | ||
|
|
||
| cell1.innerHTML = myLibrary[i].title; | ||
| cell2.innerHTML = myLibrary[i].author; | ||
| cell3.innerHTML = myLibrary[i].pages; |
There was a problem hiding this comment.
Why rewrite these statements with "less informative" variable names?
There was a problem hiding this comment.
I've made the following improvements based on your suggestions:
Removed the redundant == null checks since input .value is never null.
Added .trim() to handle whitespace-only inputs for title and author.
Improved the validation for pages to ensure it’s a positive whole number.
Replaced generic variable names like cell1 with more descriptive ones (e.g., titleCell).
Updated alert messages to give clearer instructions when inputs are invalid.
debugging/book-library/script.js
Outdated
| delBut.className = "btn btn-warning"; | ||
| delBut.innerHTML = "Delete"; | ||
| delBut.addEventListener("clicks", function () { | ||
| delButton.id = i + 5; |
There was a problem hiding this comment.
- Do you think the value assigned to the
idattribute is unique? - Do you think there is a need to assign any id attribute to
delButton? - Do you think there is a need to assign any id attribute to
changeButat line 72?
There was a problem hiding this comment.
Thanks for pointing that out! I've made the following improvements based on your feedback:
Removed unnecessary id attributes from delButton and changeBut since they weren't being used anywhere.
Simplified the logic by relying on the loop index (i) for context, which avoids potential ID conflicts and makes the code cleaner.
Updated the button logic to be more concise and easier to follow.
debugging/book-library/script.js
Outdated
|
|
||
| //delete old table | ||
| for (let n = rowsNumber - 1; n > 0; n-- { | ||
| for (let n = rowsNumber - 1; n > 0; n--) { | ||
| table.deleteRow(n); | ||
| } |
There was a problem hiding this comment.
Can you think of a more efficient way to remove all rows (except the heading) in the table?
There was a problem hiding this comment.
I’ve replaced the for loop with a more efficient approach by resetting the tbody directly. This clears all rows except the heading in a single operation, reducing DOM manipulations and improving performance. Let me know if you have further thoughts!
debugging/book-library/script.js
Outdated
| cell4.appendChild(changeBut); | ||
|
|
||
| let readStatus = myLibrary[i].check ? "Yes" : "No"; | ||
| changeBut.innerHTML = readStatus; |
There was a problem hiding this comment.
(Inconsistent variable names) Why name one variable changeBut but the other delButton?
There was a problem hiding this comment.
renamed the variables to maintain consistent and descriptive naming. changeBut is now readButton, and delButton is now deleteButton. This makes the code clearer and aligns the naming with their respective functionalities
cjyuan
added
Reviewed
- Added checks for whitespace-only inputs and invalid pages - Replaced generic variable names with meaningful ones - Enhanced error messages for better user feedback
- Simplified button logic using loop index - Cleaned up code by avoiding redundant attributes
- Replaced multiple deleteRow calls with a single tbody replacement - Optimized DOM manipulations for better performance
- changeBut → readButton - delButton → deleteButton Aligned naming with functionality for better readability.
|
Good job. I was just pointing out things that can be improved, they are not a requirement. I am impressed of your initiative and the improvements you made. Feel free to mark this as Completed any time. |
|
Thanks for your kind words @cjyuan. Was happy to learn. Just marked it as complete |
Pezhman-Azizi
added
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.